feat: model dual-era ChatGPT protocol selection - #3828
Conversation
GitHub Actions
Inactive
dosubot
Bot
added
the
size:L
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughThe change adds Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/host-config/canonicalize.ts (1)
888-937: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate stateless protocol pins against the advertised list.
The check excludes stateless pins. Therefore, this profile is accepted:
mcpProtocolVersion: "2026-07-28"withsupportedProtocolVersions: ["2025-11-25"].
MCPClientManagerthen sends the 2026 pin while it advertises only 2025 support. Validate every concrete pin when an advertised list exists. Keep the stateful-only derivation behavior after that validation.Add a canonicalization test for this conflicting 2026 pin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/host-config/canonicalize.ts` around lines 888 - 937, Update the cross-field validation in canonicalization to validate every concrete mcpProtocolVersion, including stateless versions, whenever supportedProtocolVersions or initialize.supportedProtocolVersions is present; throw the existing conflict error when the pin is absent from the advertised list. Preserve the current stateful-only derivation behavior for missing advertised lists, and add a canonicalization test covering a 2026 stateless pin conflicting with a 2025 advertised version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sdk/src/host-config/canonicalize.ts`:
- Around line 888-937: Update the cross-field validation in canonicalization to
validate every concrete mcpProtocolVersion, including stateless versions,
whenever supportedProtocolVersions or initialize.supportedProtocolVersions is
present; throw the existing conflict error when the pin is absent from the
advertised list. Preserve the current stateful-only derivation behavior for
missing advertised lists, and add a canonicalization test covering a 2026
stateless pin conflicting with a 2025 advertised version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62f7b958-05c9-425e-820c-2816c2a27af1
⛔ Files ignored due to path filters (2)
sdk/src/host-compat/catalog.generated.tsis excluded by!**/*.generated.*sdk/tests/__snapshots__/host-config-seed-host-template.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/__tests__/ProtocolTab.versionDropdown.test.tsxmcpjam-inspector/client/src/lib/__tests__/client-config-v2-mcp-profile.test.tsmcpjam-inspector/client/src/lib/client-config-v2.tsmcpjam-inspector/client/src/lib/host-config-field-schema.tssdk/src/host-config/canonicalize.tssdk/src/host-config/defaults.tssdk/src/host-config/host-connection.tssdk/src/host-config/host.tssdk/src/host-config/public-types.tssdk/src/host-config/sdk-evals-normalizer.tssdk/src/host-config/templates/seed-host-template.tssdk/src/host-config/types.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/tests/MCPClientManager.auto-protocol-reconnect.test.tssdk/tests/host-compat-market-hosts.test.tssdk/tests/host-config-canonicalize.test.tssdk/tests/host-config-sdk-evals-normalizer.test.tssdk/tests/host-connection.test.tssdk/tests/host.test.tssdk/tests/support/dual-era-fixture.test.ts
Internal previewPreview URL: https://mcp-inspector-pr-3828.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 24 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsx (2)
867-867: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winState the per-server override rule in the tooltip.
A host pin does not apply to every server. A concrete per-server protocol override takes precedence.
- title="Automatic: negotiate at connect time. Any other choice pins that exact revision for every server on this client." + title="Automatic: negotiate at connect time. Any other choice pins that exact revision unless a server has its own protocol override."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsx` at line 867, Update the tooltip text for the protocol selection control to clarify that the host-level pin applies by default, but a concrete per-server protocol override takes precedence and can select a different revision for that server.
616-627: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve complete
initialize.clientInfodata during JSON edits.
protocolToJsonemits onlynameandversion. This code then replacesinitializewith that reduced object. An unrelated JSON edit deletesclientInfofields such astitleand future-spec fields that the profile type and canonicalizer support.Serialize and parse the full
clientInforecord, while validatingnameandversion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsx` around lines 616 - 627, Update protocolToJson’s initialize handling to preserve the complete initialize.clientInfo record by serializing and parsing all clientInfo fields, rather than constructing an object containing only name and version. Validate the required name and version values during parsing, then use the preserved full record when assigning initialize.clientInfo in the next HostConfigMcpProfileV1 object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsx`:
- Line 867: Update the tooltip text for the protocol selection control to
clarify that the host-level pin applies by default, but a concrete per-server
protocol override takes precedence and can select a different revision for that
server.
- Around line 616-627: Update protocolToJson’s initialize handling to preserve
the complete initialize.clientInfo record by serializing and parsing all
clientInfo fields, rather than constructing an object containing only name and
version. Validate the required name and version values during parsing, then use
the preserved full record when assigning initialize.clientInfo in the next
HostConfigMcpProfileV1 object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee5847c8-a84c-45a7-b4c5-05ef98bf1241
⛔ Files ignored due to path filters (2)
sdk/src/host-compat/catalog.generated.tsis excluded by!**/*.generated.*sdk/tests/__snapshots__/host-config-seed-host-template.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
mcpjam-inspector/client/src/components/hosts/redesigned/focus/ProtocolTab.tsxmcpjam-inspector/client/src/components/hosts/redesigned/focus/__tests__/ProtocolTab.versionDropdown.test.tsxmcpjam-inspector/client/src/lib/__tests__/client-config-v2-mcp-profile.test.tsmcpjam-inspector/client/src/lib/host-config-field-schema.tssdk/src/host-config/canonicalize.tssdk/src/host-config/defaults.tssdk/src/host-config/host-connection.tssdk/src/host-config/public-types.tssdk/src/host-config/templates/seed-host-template.tssdk/src/host-config/types.tssdk/src/mcp-client-manager/MCPClientManager.tssdk/tests/MCPClientManager.auto-protocol-reconnect.test.tssdk/tests/host-compat-market-hosts.test.tssdk/tests/host-config-canonicalize.test.tssdk/tests/host-config-defaults.test.tssdk/tests/host-connection.test.tssdk/tests/host.test.tssdk/tests/support/dual-era-fixture.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- sdk/src/host-config/templates/seed-host-template.ts
- sdk/tests/host.test.ts
- sdk/tests/host-compat-market-hosts.test.ts
- sdk/src/mcp-client-manager/MCPClientManager.ts
- sdk/src/host-config/defaults.ts
- sdk/tests/support/dual-era-fixture.test.ts
- mcpjam-inspector/client/src/components/hosts/redesigned/focus/tests/ProtocolTab.versionDropdown.test.tsx
- sdk/tests/host-connection.test.ts
- sdk/tests/MCPClientManager.auto-protocol-reconnect.test.ts
- sdk/src/host-config/public-types.ts
There was a problem hiding this comment.
2 issues found across 24 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk/tests/host-config-sdk-evals-normalizer.test.ts">
<violation number="1">
P3: The HostJson-projection test dropped its `initialize`/`clientInfo`/`supportedProtocolVersions` assertions entirely rather than migrating them to the new nested `host.mcp.initialize` shape. Since this PR's headline is exactly that these fields now live under `initialize`, the `hostMcpToProfile` path that projects `mcp.initialize` into `mcpProfile.initialize` is left untested — consider replacing the removed setup/assertions with `host.mcp.initialize = { clientInfo: {...}, supportedProtocolVersions: [...] }` and asserting `out.mcpProfile?.initialize` to keep the new behavior covered.</violation>
</file>
<file name="sdk/src/host-config/canonicalize.ts">
<violation number="1">
P3: The new derive block re-implements the exact 5-line sorted-keys reconstruction that already exists in the `initialize` canonicalization just above it (both build a canonical `sortedInit` by iterating `Object.keys(...).sort()`). This is a small but real duplication introduced here; the two blocks can drift if the key-sorting convention ever changes. Extracting the inline sort into a tiny shared helper (e.g. `sortedRecord<T>(input)`) and reusing it in both places keeps the canonicalization path consistent and easier to audit for hash stability.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -694,12 +694,15 @@ function canonicalizeMcpProfile( | |||
|
|
|||
There was a problem hiding this comment.
P3: The new derive block re-implements the exact 5-line sorted-keys reconstruction that already exists in the initialize canonicalization just above it (both build a canonical sortedInit by iterating Object.keys(...).sort()). This is a small but real duplication introduced here; the two blocks can drift if the key-sorting convention ever changes. Extracting the inline sort into a tiny shared helper (e.g. sortedRecord<T>(input)) and reusing it in both places keeps the canonicalization path consistent and easier to audit for hash stability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sdk/src/host-config/canonicalize.ts, line 842:
<comment>The new derive block re-implements the exact 5-line sorted-keys reconstruction that already exists in the `initialize` canonicalization just above it (both build a canonical `sortedInit` by iterating `Object.keys(...).sort()`). This is a small but real duplication introduced here; the two blocks can drift if the key-sorting convention ever changes. Extracting the inline sort into a tiny shared helper (e.g. `sortedRecord<T>(input)`) and reusing it in both places keeps the canonicalization path consistent and easier to audit for hash stability.</comment>
<file context>
@@ -865,71 +820,40 @@ function canonicalizeMcpProfile(
- )[k];
- }
- out.initialize = sortedInit;
+ const sortedInit: NonNullable<HostConfigMcpProfileV1["initialize"]> = {};
+ for (const k of Object.keys(initWithDerived).sort()) {
+ (sortedInit as Record<string, unknown>)[k] = (
</file context>
2 participants
What this does
Adds ChatGPT's Automatic, MCP 2025, and MCP 2026 selection behavior to the Inspector and SDK.
clientInfoandsupportedProtocolVersionsundermcpProfile.initialize.autoas a selection policy alongside explicit 2025 and 2026 pins.initializehandshake.initialize.Related backend stack
Validation